HYPERFLEET-1371 - refactor: remove environments framework, unify startup - #327
Open
kuudori wants to merge 4 commits into
Open
HYPERFLEET-1371 - refactor: remove environments framework, unify startup#327kuudori wants to merge 4 commits into
kuudori wants to merge 4 commits into
Conversation
Replace the environments framework with direct container-based dependency injection and a linear composition root. Move tracing env vars into the Viper config system and introduce pkg/closer for ordered shutdown.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change replaces environment-driven startup with explicit configuration, dependency injection, and coordinated server shutdown. It adds tracing configuration and a concurrency-safe cleanup manager. API, health, and metrics servers share error-returning lifecycle methods. Integration tests now manage Testcontainers database setup and cleanup. Documentation, Helm values, Make targets, and test guidance reflect the new runtime model. Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ServeCommand
participant APIServer
participant HealthServer
participant MetricsServer
participant Closer
ServeCommand->>APIServer: Start()
ServeCommand->>HealthServer: Start()
ServeCommand->>MetricsServer: Start()
APIServer-->>ServeCommand: NotifyListening()
HealthServer-->>ServeCommand: NotifyListening()
MetricsServer-->>ServeCommand: NotifyListening()
ServeCommand->>APIServer: Shutdown(ctx)
ServeCommand->>HealthServer: Shutdown(ctx)
ServeCommand->>MetricsServer: Shutdown(ctx)
ServeCommand->>Closer: Close()
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 5 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 3443 lines (>500) | +2 |
| Sensitive paths | cmd/ | +2 |
| Test coverage | Missing tests for: cmd/hyperfleet-api/environments/registry cmd/hyperfleet-api/servecmd pkg/auth pkg/db/db_session test test/mocks | +1 |
Computed by hyperfleet-risk-scorer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
cmd/hyperfleet-api/server/health_server.go (1)
72-74: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCompare with
errors.Is(err, http.ErrServerClosed).Direct equality misses a wrapped sentinel.
Serve/ServeTLSreturn the bare sentinel today, but any future wrapping turns a normal shutdown into a reported failure, and cmd.go propagates that as the process exit error.metrics_server.goline 64 carries the same comparison; fix both.♻️ Proposed fix
- if err != nil && err != http.ErrServerClosed { + if err != nil && !errors.Is(err, http.ErrServerClosed) { return fmt.Errorf("health server terminated with errors: %w", err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/hyperfleet-api/server/health_server.go` around lines 72 - 74, Update the shutdown checks in the health server’s error handling and the corresponding metrics server logic to use errors.Is(err, http.ErrServerClosed) instead of direct equality, preserving normal shutdown behavior even when the sentinel is wrapped.Source: Path instructions
cmd/hyperfleet-api/servecmd/cmd.go (1)
134-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the three drain callbacks into one helper.
Lines 134-141, 144-151, and 154-161 repeat the same shape: bounded
Shutdown, join withCloseon failure.runServealready exceeds 50 lines with many branching paths, which the coding standard flags for decomposition.♻️ Proposed helper
+func addGracefulShutdown(c *closer.Closer, srv server.Server, budget time.Duration) { + c.Add(func() error { + drainCtx, cancel := context.WithTimeout(context.Background(), budget) + defer cancel() + if err := srv.Shutdown(drainCtx); err != nil { + return errors.Join(err, srv.Close()) + } + return nil + }) +}Then at the call sites:
- c.Add(func() error { - drainCtx, cancel := context.WithTimeout(context.Background(), cfg.Health.ShutdownTimeout) - defer cancel() - if err := apiServer.Shutdown(drainCtx); err != nil { - return errors.Join(err, apiServer.Close()) - } - return nil - }) + addGracefulShutdown(c, apiServer, cfg.Health.ShutdownTimeout) metricsServer := server.NewMetricsServer(cfg.Metrics) - c.Add(func() error { - drainCtx, cancel := context.WithTimeout(context.Background(), metricsDrainTimeout) - defer cancel() - if err := metricsServer.Shutdown(drainCtx); err != nil { - return errors.Join(err, metricsServer.Close()) - } - return nil - }) + addGracefulShutdown(c, metricsServer, metricsDrainTimeout) healthServer := server.NewHealthServer(cfg.Health, ctr.SessionFactory()) - c.Add(func() error { - drainCtx, cancel := context.WithTimeout(context.Background(), healthDrainTimeout) - defer cancel() - if err := healthServer.Shutdown(drainCtx); err != nil { - return errors.Join(err, healthServer.Close()) - } - return nil - }) + addGracefulShutdown(c, healthServer, healthDrainTimeout)Keep the existing comment at lines 131-133 above the helper so the "never register
Closebare" rule stays documented.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/hyperfleet-api/servecmd/cmd.go` around lines 134 - 161, Extract the repeated shutdown-and-close logic from the three callbacks in runServe into one helper that accepts the server, drain timeout, and returns the bounded Shutdown error joined with Close on failure. Register each callback through this helper for apiServer, metricsServer, and healthServer, while preserving the existing comment above the helper documenting why Close must not be registered bare.Source: Path instructions
cmd/hyperfleet-api/container/db.go (1)
8-17: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake
SetSessionFactoryreject or close an already-constructed factory.
SessionFactory()caches a production factory on first call.SetSessionFactorythen overwrites that field without closing the previous value. If any code path touchesSessionFactory()before injection (test harness, future wiring), the process opens a production connection pool that nobody closes and nobody can reach.The lazy assignment is also unsynchronized. Today the reviewed callers invoke it on the main goroutine before servers start, so no race is proven; keep it that way or add a mutex if any getter moves onto a request path.
♻️ Proposed guard
func (c *Container) SetSessionFactory(sf db.SessionFactory) { + if c.sessionFactory != nil { + panic("container: session factory already constructed; inject before first use") + } c.sessionFactory = sf }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/hyperfleet-api/container/db.go` around lines 8 - 17, Update Container.SetSessionFactory to handle an existing cached factory before replacing it: reject the replacement or close the previously constructed factory so a production factory created by SessionFactory is never orphaned. Preserve the lazy caching behavior in SessionFactory, and keep access serialized as currently assumed or add synchronization if the getter is moved to a concurrent request path.Source: Coding guidelines
pkg/config/logging_test.go (1)
44-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise tracing overrides instead of only the default.
The test body does not set a tracing-specific environment value, so
Tracing.Enabled == truemay only verifyNewTracingConfig()'s default. Add table-driven cases forHYPERFLEET_TRACING_ENABLED,HYPERFLEET_TRACING_SERVICE_NAME, andOTEL_SERVICE_NAME, including precedence when both service-name variables are set. Rename the test to reflect its tracing coverage.As per path instructions:
**/*_test.gorequires tests for new critical configuration paths and favors table-driven scenarios.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/config/logging_test.go` at line 44, Rename the tracing configuration test to reflect override coverage and convert it to table-driven cases. Exercise HYPERFLEET_TRACING_ENABLED, HYPERFLEET_TRACING_SERVICE_NAME, and OTEL_SERVICE_NAME, including the expected precedence when both service-name variables are set, while retaining assertions for the resulting Tracing fields.Source: Path instructions
test/testdata/integration-config.yaml (1)
4-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the runtime overrides in this fixture.
jwk_cert_urlis a placeholder.test/helper.goline 175 replaces it with the JWK mock URL.identity_headeris absent here andtest/helper.golines 223-227 injectsdefaultTestIdentityHeader. Both couplings are invisible to a reader of this file. Add comments, and setidentity_headerexplicitly so the fixture matches what the suite runs.Proposed fixture annotation
server: jwt: enabled: true configs: + # jwk_cert_url is replaced at runtime by the JWK mock server URL (test/helper.go). - issuer_url: https://test-issuer.example.com jwk_cert_url: https://jwks.invalid/.well-known/jwks.json header: Authorization identity_claim: email + identity_header: X-Hyperfleet-IdentityMatch
identity_headerto the value ofdefaultTestIdentityHeader.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/testdata/integration-config.yaml` around lines 4 - 8, Update the integration config fixture’s config entry to include identity_header with the value defined by defaultTestIdentityHeader, and add comments documenting that jwk_cert_url is replaced by the JWK mock URL and identity_header is injected or overridden by test/helper.go at runtime. Keep the fixture values aligned with the suite’s effective runtime configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Line 1: Update the top-level heading in AGENTS.md from “CLAUDE.md” to
“AGENTS.md” so the document identifies itself correctly.
In `@cmd/hyperfleet-api/servecmd/cmd.go`:
- Around line 186-201: Update the startup select flow around
healthServer.NotifyListening and serverResults to also await an API
listener-ready signal from APIServer.Start. Track health and API listening
independently, and call health.GetReadinessState().SetReady only after both
signals have completed; preserve context, signal, and startup-error handling
while ensuring API binding failures prevent readiness.
In `@cmd/hyperfleet-api/server/api_server.go`:
- Around line 46-50: Update the invalid-TLS branch in the server startup flow to
handle the error returned by listener.Close instead of discarding it. Combine or
otherwise propagate the close error with the existing certificate/key
configuration error while preserving the current cleanup and failure behavior.
In `@cmd/hyperfleet-api/server/health_server.go`:
- Around line 36-40: Configure ReadTimeout, WriteTimeout, and IdleTimeout on the
http.Server instances created by NewHealthServer in
cmd/hyperfleet-api/server/health_server.go (lines 36-40) and NewMetricsServer in
cmd/hyperfleet-api/server/metrics_server.go (lines 30-34), using the project’s
appropriate timeout values while preserving the existing handlers and addresses.
In `@Makefile`:
- Around line 216-234: Update each gotestsum invocation in the test targets
around ci-test-unit and ci-test-integration, including the corresponding regular
unit and integration targets, to run with CGO_ENABLED=1 and
GOEXPERIMENT=boringcrypto. Apply both environment variables directly to every
command so the install prerequisite does not determine the test binary build
configuration.
In `@pkg/config/health.go`:
- Around line 67-69: Rename HealthConfig.GetDBPingTimeout to PingTimeout, then
update the health-server interface and every call site to use the new method
while preserving its existing return value. Do not use DBPingTimeout, which
conflicts with the struct field.
In `@pkg/config/loader.go`:
- Line 312: Handle the error returned by BindEnv in bindAllEnvVars instead of
suppressing it with nolint. Propagate the error by updating bindAllEnvVars and
its callers as needed, or explicitly fail fast after checking it, while
preserving the existing environment binding behavior.
In `@test/helper.go`:
- Around line 171-191: Remove the zero-value testing.T dependency from the
helper setup around NewJWKCertServerMock and Helper.T. Update the JWK mock error
path to return an HTTP error response instead of calling methods on testing.T,
and eliminate any reliance on the &testing.T{} instance while preserving normal
test failure handling.
---
Nitpick comments:
In `@cmd/hyperfleet-api/container/db.go`:
- Around line 8-17: Update Container.SetSessionFactory to handle an existing
cached factory before replacing it: reject the replacement or close the
previously constructed factory so a production factory created by SessionFactory
is never orphaned. Preserve the lazy caching behavior in SessionFactory, and
keep access serialized as currently assumed or add synchronization if the getter
is moved to a concurrent request path.
In `@cmd/hyperfleet-api/servecmd/cmd.go`:
- Around line 134-161: Extract the repeated shutdown-and-close logic from the
three callbacks in runServe into one helper that accepts the server, drain
timeout, and returns the bounded Shutdown error joined with Close on failure.
Register each callback through this helper for apiServer, metricsServer, and
healthServer, while preserving the existing comment above the helper documenting
why Close must not be registered bare.
In `@cmd/hyperfleet-api/server/health_server.go`:
- Around line 72-74: Update the shutdown checks in the health server’s error
handling and the corresponding metrics server logic to use errors.Is(err,
http.ErrServerClosed) instead of direct equality, preserving normal shutdown
behavior even when the sentinel is wrapped.
In `@pkg/config/logging_test.go`:
- Line 44: Rename the tracing configuration test to reflect override coverage
and convert it to table-driven cases. Exercise HYPERFLEET_TRACING_ENABLED,
HYPERFLEET_TRACING_SERVICE_NAME, and OTEL_SERVICE_NAME, including the expected
precedence when both service-name variables are set, while retaining assertions
for the resulting Tracing fields.
In `@test/testdata/integration-config.yaml`:
- Around line 4-8: Update the integration config fixture’s config entry to
include identity_header with the value defined by defaultTestIdentityHeader, and
add comments documenting that jwk_cert_url is replaced by the JWK mock URL and
identity_header is injected or overridden by test/helper.go at runtime. Keep the
fixture values aligned with the suite’s effective runtime configuration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6316064b-2ef8-41bf-8185-a2be89858e69
⛔ Files ignored due to path filters (1)
test/support/jwt_ca.pemis excluded by!**/*.pem
📒 Files selected for processing (49)
AGENTS.mdCLAUDE.mdCONTRIBUTING.mdMakefilecharts/README.mdcharts/templates/configmap.yamlcharts/values.yamlcmd/hyperfleet-api/container/auth.gocmd/hyperfleet-api/container/container.gocmd/hyperfleet-api/container/container_test.gocmd/hyperfleet-api/container/daos.gocmd/hyperfleet-api/container/db.gocmd/hyperfleet-api/container/validation.gocmd/hyperfleet-api/environments/e_development.gocmd/hyperfleet-api/environments/e_integration_testing.gocmd/hyperfleet-api/environments/e_production.gocmd/hyperfleet-api/environments/e_unit_testing.gocmd/hyperfleet-api/environments/framework.gocmd/hyperfleet-api/environments/framework_test.gocmd/hyperfleet-api/environments/registry/registry.gocmd/hyperfleet-api/environments/types.gocmd/hyperfleet-api/servecmd/api_server.gocmd/hyperfleet-api/servecmd/cmd.gocmd/hyperfleet-api/server/api_server.gocmd/hyperfleet-api/server/api_server_test.gocmd/hyperfleet-api/server/health_server.gocmd/hyperfleet-api/server/metrics_server.gocmd/hyperfleet-api/server/routes_entities.gocmd/hyperfleet-api/server/server.godocs/authentication.mddocs/deployment.mddocs/development.mddocs/logging.mddocs/testcontainers.mdpkg/closer/closer.gopkg/closer/closer_test.gopkg/config/config.gopkg/config/dump.gopkg/config/health.gopkg/config/loader.gopkg/config/logging.gopkg/config/logging_test.gopkg/config/metrics.gopkg/config/tracing.gotest/CLAUDE.mdtest/helper.gotest/integration/caller_identity_test.gotest/support/certs.jsontest/testdata/integration-config.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (10)
- cmd/hyperfleet-api/environments/e_integration_testing.go
- test/support/certs.json
- pkg/config/logging.go
- cmd/hyperfleet-api/environments/e_production.go
- cmd/hyperfleet-api/environments/types.go
- cmd/hyperfleet-api/environments/framework.go
- cmd/hyperfleet-api/environments/e_development.go
- cmd/hyperfleet-api/environments/framework_test.go
- cmd/hyperfleet-api/environments/registry/registry.go
- cmd/hyperfleet-api/environments/e_unit_testing.go
- Use errors.Is for http.ErrServerClosed in all three servers - Extract addDrain helper for repeated shutdown callbacks - Add table-driven tracing config env var tests - Add identity_header to integration config fixture
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/helper.go (1)
230-235: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCall
Closeonly after a drain timeout.Line 234 force-closes the API server for every
Shutdownerror. Return unexpected shutdown errors. CallCloseonly whenShutdowntimed out.Proposed fix
if shutdownErr := helper.APIServer.Shutdown(drainCtx); shutdownErr != nil { - return errors.Join(shutdownErr, helper.APIServer.Close()) + if errors.Is(shutdownErr, context.DeadlineExceeded) { + return errors.Join(shutdownErr, helper.APIServer.Close()) + } + return shutdownErr }As per coding guidelines, "
Close()only as the force-close fallback after shutdown times out."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/helper.go` around lines 230 - 235, Update the helper.closer cleanup callback around helper.APIServer.Shutdown so Close is invoked only when the shutdown error indicates the drain context timed out; return other shutdown errors directly without force-closing, while preserving normal successful shutdown behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/hyperfleet-api/server/routes_entities.go`:
- Around line 43-44: Update the error handling around registerPerEntityRoutes in
RegisterEntityRoutes to wrap the returned error with a message identifying
RegisterEntityRoutes as the failed operation, using %w to preserve the original
cause instead of returning err directly.
In `@cmd/hyperfleet-api/server/server.go`:
- Around line 63-69: Update baseServer.Start so the configured TLS certificate
and key are loaded and validated before closing s.listening. Ensure
certificate/key errors return from startup and prevent NotifyListening
publication, while preserving normal listener serving after successful
validation.
In `@pkg/config/logging_test.go`:
- Around line 75-78: Extend the service-name precedence table in the logging
configuration tests to set both HYPERFLEET_TRACING_SERVICE_NAME and
OTEL_SERVICE_NAME, expecting the OTEL_SERVICE_NAME value. Preserve the existing
single-variable coverage and use the established test-case structure.
In `@test/integration/integration_test.go`:
- Around line 109-121: Ensure the integration setup immediately defers or
centralizes termination of pgContainer after postgres.Run succeeds, and route
every subsequent failure through that cleanup path. Handle errors from Host,
MappedPort, and both os.Setenv calls before exiting, terminating pgContainer
before os.Exit(1) on each failure.
In `@test/mocks/jwk_cert_server.go`:
- Line 36: Check and handle the error returned by fmt.Fprintf in the JWK
response handler before returning, using the existing handler’s error-handling
conventions and ensuring failed client writes are not discarded.
---
Outside diff comments:
In `@test/helper.go`:
- Around line 230-235: Update the helper.closer cleanup callback around
helper.APIServer.Shutdown so Close is invoked only when the shutdown error
indicates the drain context timed out; return other shutdown errors directly
without force-closing, while preserving normal successful shutdown behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0688a445-1dd5-4a32-b4b5-e5d4c12aaf11
📒 Files selected for processing (21)
AGENTS.mdcmd/hyperfleet-api/container/db.gocmd/hyperfleet-api/servecmd/cmd.gocmd/hyperfleet-api/server/api_server.gocmd/hyperfleet-api/server/health_server.gocmd/hyperfleet-api/server/metrics_server.gocmd/hyperfleet-api/server/routes_entities.gocmd/hyperfleet-api/server/server.gopkg/auth/auth_middleware.gopkg/auth/identity.gopkg/auth/jwt_handler.gopkg/config/health.gopkg/config/loader.gopkg/config/logging_test.gopkg/db/db_session/testcontainer.gotest/helper.gotest/integration/advisory_locks_test.gotest/integration/integration_test.gotest/mocks/jwk_cert_server.gotest/registration.gotest/testdata/integration-config.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (4)
- pkg/db/db_session/testcontainer.go
- cmd/hyperfleet-api/container/db.go
- pkg/auth/auth_middleware.go
- pkg/auth/identity.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/config/health.go
- AGENTS.md
- cmd/hyperfleet-api/servecmd/cmd.go
| if _, err := fmt.Fprintf(w, `{"keys":[%s]}`, string(jwkBytes)); err != nil { | ||
| t.Errorf("error writing jwk response: %v", err) | ||
| } | ||
| fmt.Fprintf(w, `{"keys":[%s]}`, string(jwkBytes)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Check the response write error.
Line 36 discards the error from fmt.Fprintf. A failed client write is an exceptional condition. Check it before returning. This is CWE-703.
Proposed fix
- fmt.Fprintf(w, `{"keys":[%s]}`, string(jwkBytes))
+ if _, err := fmt.Fprintf(w, `{"keys":[%s]}`, string(jwkBytes)); err != nil {
+ return
+ }As per path instructions, "every error return MUST be checked."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fmt.Fprintf(w, `{"keys":[%s]}`, string(jwkBytes)) | |
| if _, err := fmt.Fprintf(w, `{"keys":[%s]}`, string(jwkBytes)); err != nil { | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/mocks/jwk_cert_server.go` at line 36, Check and handle the error
returned by fmt.Fprintf in the JWK response handler before returning, using the
existing handler’s error-handling conventions and ensuring failed client writes
are not discarded.
Source: Path instructions
Contributor
Author
There was a problem hiding this comment.
Test mock HTTP handler writing to a test client
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f '.*_test\.go$' test/mocks test 2>/dev/null | sort
printf '\n--- JWK mock call sites ---\n'
rg -n -C 4 '\bNewJWKCertServerMock\s*\(' test
printf '\n--- mock test outlines ---\n'
fd -t f '.*_test\.go$' test/mocks -x ast-grep outline {} --items allLength of output: 2381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- helper JWK setup ---'
sed -n '120,180p' test/helper.go
printf '%s\n' '--- module Go version and JWK dependency ---'
rg -n -C 2 '^(go |.*gojwk)' go.mod go.sum
printf '%s\n' '--- existing RSA key generation in tests ---'
rg -n -C 3 '\brsa\.GenerateKey\s*\(' test pkg cmdLength of output: 12401
@kuudori, add test/mocks/jwk_cert_server_test.go with this success-path test:
package mocks
import (
"crypto/rand"
"crypto/rsa"
"encoding/json"
"net/http"
"testing"
)
func TestNewJWKCertServerMockServesJWK(t *testing.T) {
privateKey, err := rsa.GenerateKey(rand.Reader, 2048)
if err != nil {
t.Fatalf("generate RSA key: %v", err)
}
url, teardown := NewJWKCertServerMock(&privateKey.PublicKey, "test-kid", "RS256")
t.Cleanup(func() {
if err := teardown(); err != nil {
t.Errorf("close JWK server: %v", err)
}
})
response, err := http.Get(url)
if err != nil {
t.Fatalf("request JWK endpoint: %v", err)
}
defer response.Body.Close()
if response.StatusCode != http.StatusOK {
t.Fatalf("unexpected status: got %d, want %d", response.StatusCode, http.StatusOK)
}
var jwks struct {
Keys []struct {
Kid string `json:"kid"`
Alg string `json:"alg"`
} `json:"keys"`
}
if err := json.NewDecoder(response.Body).Decode(&jwks); err != nil {
t.Fatalf("decode JWK response: %v", err)
}
if len(jwks.Keys) != 1 {
t.Fatalf("unexpected key count: got %d, want 1", len(jwks.Keys))
}
if jwks.Keys[0].Kid != "test-kid" {
t.Errorf("unexpected key ID: got %q, want %q", jwks.Keys[0].Kid, "test-kid")
}
if jwks.Keys[0].Alg != "RS256" {
t.Errorf("unexpected algorithm: got %q, want %q", jwks.Keys[0].Alg, "RS256")
}
}This test validates the response write to an HTTP test client. It does not exercise a failed fmt.Fprintf write, because an ordinary HTTP client does not reliably force http.ResponseWriter.Write to fail.
You are interacting with an AI system.
kuudori
force-pushed
the
HYPERFLEET-1371-remove-environments
branch
from
August 6, 2026 14:28
50b1ddd to
0b0c4f8
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
cmd/hyperfleet-api/server/server.go (1)
63-70: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReadiness is published before the TLS material is validated.
Start()closess.listeningat Line 68.Serve()then fails on two paths: the empty cert/key guard at Lines 42-48, and aServeTLSfailure on an unreadable or mismatched key pair. In both cases readiness is already signaled, soservecmdcan mark the process ready while the API server never serves traffic. Load the key pair withtls.LoadX509KeyPairbefore you closes.listening, and return the error fromStart().This repeats a finding from an earlier commit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/hyperfleet-api/server/server.go` around lines 63 - 70, The Start method publishes readiness before TLS credentials are validated. Load and validate the certificate/key pair with tls.LoadX509KeyPair before closing s.listening, return any validation error from Start, and ensure Serve/ServeTLS reuses the validated material without signaling readiness when startup fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/closer/closer_test.go`:
- Line 170: Update the cleanup call in the closer test to assert that c.Close()
succeeds using the test framework’s expectation, replacing the discarded return
value while preserving the existing cleanup flow.
- Line 16: Update the test loop around c.Close() to capture and assert that
Close returns no error, rather than discarding its result. Preserve the existing
iteration behavior and test expectations.
In `@test/helper.go`:
- Around line 146-160: The setup helper leaks resources because cleanup is
registered only after later panic points in the same flow. Update the logic
around ctr.SessionFactory(), db.Migrate, NewJWKCertServerMock, and closer.New()
so the closer is created first and each teardown (ctr.SessionFactory().Close and
jwkTeardown) is added immediately after the corresponding resource is created,
before any validation or panic can occur. Keep the existing setup behavior
unchanged otherwise.
- Around line 440-466: Update orderTablesByDependencies to skip foreign-key
edges whose TableName equals ReferencedName or whose ReferencedName is not
present in the requested tables/dependencies map, before appending the
dependency. Preserve system-table filtering and ensure only in-scope,
non-self-referencing edges affect ordering and subsequent DropTable calls.
- Around line 189-200: Update the Helper startup cleanup registration so
pgContainer termination is added to helper.closer before any startup failure can
invoke failStartup. Ensure failStartup continues closing helper.closer and
exiting via os.Exit(1), without relying on post-NewHelper cleanup or replacing
the exit with a panic.
---
Duplicate comments:
In `@cmd/hyperfleet-api/server/server.go`:
- Around line 63-70: The Start method publishes readiness before TLS credentials
are validated. Load and validate the certificate/key pair with
tls.LoadX509KeyPair before closing s.listening, return any validation error from
Start, and ensure Serve/ServeTLS reuses the validated material without signaling
readiness when startup fails.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b8c793d1-c689-425b-8297-2e47f4be5841
⛔ Files ignored due to path filters (1)
test/support/jwt_ca.pemis excluded by!**/*.pem
📒 Files selected for processing (57)
AGENTS.mdCLAUDE.mdCONTRIBUTING.mdMakefilecharts/README.mdcharts/templates/configmap.yamlcharts/values.yamlcmd/hyperfleet-api/container/auth.gocmd/hyperfleet-api/container/container.gocmd/hyperfleet-api/container/container_test.gocmd/hyperfleet-api/container/daos.gocmd/hyperfleet-api/container/db.gocmd/hyperfleet-api/container/validation.gocmd/hyperfleet-api/environments/e_development.gocmd/hyperfleet-api/environments/e_integration_testing.gocmd/hyperfleet-api/environments/e_production.gocmd/hyperfleet-api/environments/e_unit_testing.gocmd/hyperfleet-api/environments/framework.gocmd/hyperfleet-api/environments/framework_test.gocmd/hyperfleet-api/environments/registry/registry.gocmd/hyperfleet-api/environments/types.gocmd/hyperfleet-api/servecmd/api_server.gocmd/hyperfleet-api/servecmd/cmd.gocmd/hyperfleet-api/server/api_server.gocmd/hyperfleet-api/server/api_server_test.gocmd/hyperfleet-api/server/health_server.gocmd/hyperfleet-api/server/metrics_server.gocmd/hyperfleet-api/server/routes_entities.gocmd/hyperfleet-api/server/server.godocs/authentication.mddocs/deployment.mddocs/development.mddocs/logging.mddocs/testcontainers.mdpkg/auth/auth_middleware.gopkg/auth/identity.gopkg/auth/jwt_handler.gopkg/closer/closer.gopkg/closer/closer_test.gopkg/config/config.gopkg/config/dump.gopkg/config/health.gopkg/config/loader.gopkg/config/logging.gopkg/config/logging_test.gopkg/config/metrics.gopkg/config/tracing.gopkg/db/db_session/testcontainer.gotest/CLAUDE.mdtest/helper.gotest/integration/advisory_locks_test.gotest/integration/caller_identity_test.gotest/integration/integration_test.gotest/mocks/jwk_cert_server.gotest/registration.gotest/support/certs.jsontest/testdata/integration-config.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (13)
- pkg/auth/identity.go
- cmd/hyperfleet-api/environments/e_integration_testing.go
- cmd/hyperfleet-api/environments/framework_test.go
- pkg/auth/auth_middleware.go
- test/support/certs.json
- cmd/hyperfleet-api/environments/registry/registry.go
- cmd/hyperfleet-api/environments/e_production.go
- cmd/hyperfleet-api/environments/e_development.go
- pkg/db/db_session/testcontainer.go
- pkg/config/logging.go
- cmd/hyperfleet-api/environments/types.go
- cmd/hyperfleet-api/environments/e_unit_testing.go
- cmd/hyperfleet-api/environments/framework.go
🚧 Files skipped from review as they are similar to previous changes (39)
- pkg/auth/jwt_handler.go
- docs/logging.md
- cmd/hyperfleet-api/container/db.go
- CLAUDE.md
- pkg/config/dump.go
- pkg/config/health.go
- pkg/config/tracing.go
- test/integration/advisory_locks_test.go
- test/testdata/integration-config.yaml
- docs/authentication.md
- pkg/config/metrics.go
- docs/deployment.md
- charts/README.md
- cmd/hyperfleet-api/server/metrics_server.go
- pkg/config/config.go
- cmd/hyperfleet-api/container/auth.go
- AGENTS.md
- test/integration/integration_test.go
- charts/values.yaml
- cmd/hyperfleet-api/container/container_test.go
- test/mocks/jwk_cert_server.go
- cmd/hyperfleet-api/servecmd/api_server.go
- cmd/hyperfleet-api/container/daos.go
- charts/templates/configmap.yaml
- cmd/hyperfleet-api/server/api_server_test.go
- docs/development.md
- cmd/hyperfleet-api/servecmd/cmd.go
- test/CLAUDE.md
- pkg/config/logging_test.go
- test/registration.go
- test/integration/caller_identity_test.go
- pkg/closer/closer.go
- CONTRIBUTING.md
- cmd/hyperfleet-api/server/health_server.go
- cmd/hyperfleet-api/container/validation.go
- cmd/hyperfleet-api/container/container.go
- docs/testcontainers.md
- cmd/hyperfleet-api/server/routes_entities.go
- pkg/config/loader.go
kuudori
force-pushed
the
HYPERFLEET-1371-remove-environments
branch
from
August 6, 2026 14:39
0b0c4f8 to
ce59e93
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
kuudori
force-pushed
the
HYPERFLEET-1371-remove-environments
branch
from
August 6, 2026 15:41
ce59e93 to
e975068
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pkg/config/tracing.go (1)
3-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate and wire
TracingConfig
validateConfigdoes not callTracing.Validate(). Add validation for an emptyServiceNamewhen tracing is enabled, call it fromvalidateConfig, and add tests. This prevents invalidservice.namevalues (CWE-20).Align the default: code, tests, and documentation use
true, while the architecture standard and Helm chart usefalse.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/config/tracing.go` around lines 3 - 13, Update TracingConfig by adding a Validate method that rejects an empty ServiceName when Enabled is true, then invoke Tracing.Validate from validateConfig and add coverage for both valid and invalid configurations. Reconcile the tracing Enabled default across NewTracingConfig, tests, documentation, the architecture standard, and Helm chart so every source uses the intended consistent value.Sources: Path instructions, Linked repositories
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/hyperfleet-api/server/api_server_test.go`:
- Around line 156-169: In cmd/hyperfleet-api/server/api_server_test.go:156-169,
synchronize on listener readiness with a bounded assertion before launching the
blocking request, retain and assert both Serve and request errors, and after
closing the server wait for both goroutines to complete. Apply the same
readiness synchronization, error retention/checking, and goroutine joins in
cmd/hyperfleet-api/server/api_server_test.go:201-206 so every asynchronous
operation has a bounded shutdown path.
In `@cmd/hyperfleet-api/server/server.go`:
- Line 52: Avoid reloading TLS files in the serving path: retain the certificate
validated around the existing TLS setup in httpServer.TLSConfig.Certificates,
then call ServeTLS with empty certificate and key paths. Ensure the same
validated certificate is used before readiness publication and preserve the
existing listener shutdown behavior.
In `@test/helper.go`:
- Line 375: Update the cached table construction used by getAllTables and the
TRUNCATE execution to use PostgreSQL identifier quoting via quote_ident, rather
than Go’s %q formatting. Ensure table names containing embedded double quotes
are escaped as PostgreSQL requires, and remove the existing %q-based quoting
loop while preserving the CASCADE truncation behavior.
- Around line 359-379: Synchronize all access to the package-level cachedTables
state: in test/helper.go lines 359-379, protect the nil-check and assignment in
Helper.ResetDB with a shared mutex (or initialize it through the existing
sync.Once in NewHelper); in test/helper.go lines 508-514, acquire that same
mutex before RebuildSchema clears cachedTables. Ensure both mutation paths use
the same synchronization mechanism.
---
Nitpick comments:
In `@pkg/config/tracing.go`:
- Around line 3-13: Update TracingConfig by adding a Validate method that
rejects an empty ServiceName when Enabled is true, then invoke Tracing.Validate
from validateConfig and add coverage for both valid and invalid configurations.
Reconcile the tracing Enabled default across NewTracingConfig, tests,
documentation, the architecture standard, and Helm chart so every source uses
the intended consistent value.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6876c5f3-d284-48d1-b0ec-bb9e0c60ec39
⛔ Files ignored due to path filters (1)
test/support/jwt_ca.pemis excluded by!**/*.pem
📒 Files selected for processing (57)
AGENTS.mdCLAUDE.mdCONTRIBUTING.mdMakefilecharts/README.mdcharts/templates/configmap.yamlcharts/values.yamlcmd/hyperfleet-api/container/auth.gocmd/hyperfleet-api/container/container.gocmd/hyperfleet-api/container/container_test.gocmd/hyperfleet-api/container/daos.gocmd/hyperfleet-api/container/db.gocmd/hyperfleet-api/container/validation.gocmd/hyperfleet-api/environments/e_development.gocmd/hyperfleet-api/environments/e_integration_testing.gocmd/hyperfleet-api/environments/e_production.gocmd/hyperfleet-api/environments/e_unit_testing.gocmd/hyperfleet-api/environments/framework.gocmd/hyperfleet-api/environments/framework_test.gocmd/hyperfleet-api/environments/registry/registry.gocmd/hyperfleet-api/environments/types.gocmd/hyperfleet-api/servecmd/api_server.gocmd/hyperfleet-api/servecmd/cmd.gocmd/hyperfleet-api/server/api_server.gocmd/hyperfleet-api/server/api_server_test.gocmd/hyperfleet-api/server/health_server.gocmd/hyperfleet-api/server/metrics_server.gocmd/hyperfleet-api/server/routes_entities.gocmd/hyperfleet-api/server/server.godocs/authentication.mddocs/deployment.mddocs/development.mddocs/logging.mddocs/testcontainers.mdpkg/auth/auth_middleware.gopkg/auth/identity.gopkg/auth/jwt_handler.gopkg/closer/closer.gopkg/closer/closer_test.gopkg/config/config.gopkg/config/dump.gopkg/config/health.gopkg/config/loader.gopkg/config/logging.gopkg/config/logging_test.gopkg/config/metrics.gopkg/config/tracing.gopkg/db/db_session/testcontainer.gotest/CLAUDE.mdtest/helper.gotest/integration/advisory_locks_test.gotest/integration/caller_identity_test.gotest/integration/integration_test.gotest/mocks/jwk_cert_server.gotest/registration.gotest/support/certs.jsontest/testdata/integration-config.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (12)
- pkg/auth/identity.go
- pkg/db/db_session/testcontainer.go
- cmd/hyperfleet-api/environments/registry/registry.go
- cmd/hyperfleet-api/environments/types.go
- cmd/hyperfleet-api/environments/e_production.go
- test/support/certs.json
- cmd/hyperfleet-api/environments/e_integration_testing.go
- cmd/hyperfleet-api/environments/framework_test.go
- pkg/auth/auth_middleware.go
- cmd/hyperfleet-api/environments/e_development.go
- cmd/hyperfleet-api/environments/framework.go
- cmd/hyperfleet-api/environments/e_unit_testing.go
🚧 Files skipped from review as they are similar to previous changes (40)
- cmd/hyperfleet-api/container/validation.go
- docs/deployment.md
- CLAUDE.md
- cmd/hyperfleet-api/container/db.go
- test/registration.go
- CONTRIBUTING.md
- cmd/hyperfleet-api/container/daos.go
- Makefile
- test/integration/caller_identity_test.go
- charts/values.yaml
- docs/logging.md
- pkg/auth/jwt_handler.go
- test/integration/integration_test.go
- charts/README.md
- docs/authentication.md
- docs/development.md
- pkg/closer/closer.go
- pkg/config/dump.go
- cmd/hyperfleet-api/container/container.go
- test/integration/advisory_locks_test.go
- charts/templates/configmap.yaml
- cmd/hyperfleet-api/container/auth.go
- test/testdata/integration-config.yaml
- cmd/hyperfleet-api/container/container_test.go
- pkg/config/config.go
- cmd/hyperfleet-api/server/routes_entities.go
- cmd/hyperfleet-api/server/metrics_server.go
- test/CLAUDE.md
- pkg/config/metrics.go
- cmd/hyperfleet-api/servecmd/api_server.go
- docs/testcontainers.md
- test/mocks/jwk_cert_server.go
- cmd/hyperfleet-api/server/health_server.go
- pkg/config/logging_test.go
- pkg/config/loader.go
- pkg/closer/closer_test.go
- AGENTS.md
- cmd/hyperfleet-api/server/api_server.go
- cmd/hyperfleet-api/servecmd/cmd.go
- pkg/config/health.go
kuudori
force-pushed
the
HYPERFLEET-1371-remove-environments
branch
from
August 6, 2026 15:56
e975068 to
29bb6c5
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/helper.go (1)
413-427: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReturn raw table names from
getAllTablesquote_identcan return quoted text for non-simple names. This breaks dependency matching and causes GORM'sDropTableto quote the value again. Keep raw names in the slice and quote them only in theTRUNCATEstatement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/helper.go` around lines 413 - 427, Update Helper.getAllTables to select raw tablename values instead of applying quote_ident, so dependency matching and GORM DropTable receive unquoted names. Preserve the existing systemTables filter and ordering, and retain identifier quoting only where names are used by the TRUNCATE statement.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Line 66: Update the Startup wiring entry in AGENTS.md to remove the obsolete
servecmd.runServe sequence, name the current linear composition-root entry point
shown by the PR, and include pkg/closer shutdown wiring with LIFO ordering.
Preserve the existing startup component sequence where still applicable.
In `@cmd/hyperfleet-api/server/server.go`:
- Around line 85-90: Update baseServer.Shutdown and baseServer.Close to wrap
returned HTTP server errors with the server identity and operation name,
following the project's Error Model Standard rather than returning raw errors.
Preserve nil success behavior and ensure aggregated pkg/closer failures identify
which server and whether shutdown or close failed.
In `@pkg/config/tracing.go`:
- Line 19: Update the Enabled default in NewTracingConfig to false so
NewApplicationConfig produces disabled tracing unless explicitly enabled, while
preserving explicit configuration overrides.
In `@test/helper.go`:
- Around line 146-159: Update the setup failure paths in the surrounding test
helper to call c.Close() immediately before each panic: the migration error
branch after db.Migrate and the missing-JWT-config branch after
NewJWKCertServerMock. Preserve the existing panic messages and ensure cleanup
covers both the container session factory and JWK teardown.
---
Outside diff comments:
In `@test/helper.go`:
- Around line 413-427: Update Helper.getAllTables to select raw tablename values
instead of applying quote_ident, so dependency matching and GORM DropTable
receive unquoted names. Preserve the existing systemTables filter and ordering,
and retain identifier quoting only where names are used by the TRUNCATE
statement.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0a97ef61-d14f-4f0b-8ee7-95ff31c51613
⛔ Files ignored due to path filters (1)
test/support/jwt_ca.pemis excluded by!**/*.pem
📒 Files selected for processing (57)
AGENTS.mdCLAUDE.mdCONTRIBUTING.mdMakefilecharts/README.mdcharts/templates/configmap.yamlcharts/values.yamlcmd/hyperfleet-api/container/auth.gocmd/hyperfleet-api/container/container.gocmd/hyperfleet-api/container/container_test.gocmd/hyperfleet-api/container/daos.gocmd/hyperfleet-api/container/db.gocmd/hyperfleet-api/container/validation.gocmd/hyperfleet-api/environments/e_development.gocmd/hyperfleet-api/environments/e_integration_testing.gocmd/hyperfleet-api/environments/e_production.gocmd/hyperfleet-api/environments/e_unit_testing.gocmd/hyperfleet-api/environments/framework.gocmd/hyperfleet-api/environments/framework_test.gocmd/hyperfleet-api/environments/registry/registry.gocmd/hyperfleet-api/environments/types.gocmd/hyperfleet-api/servecmd/api_server.gocmd/hyperfleet-api/servecmd/cmd.gocmd/hyperfleet-api/server/api_server.gocmd/hyperfleet-api/server/api_server_test.gocmd/hyperfleet-api/server/health_server.gocmd/hyperfleet-api/server/metrics_server.gocmd/hyperfleet-api/server/routes_entities.gocmd/hyperfleet-api/server/server.godocs/authentication.mddocs/deployment.mddocs/development.mddocs/logging.mddocs/testcontainers.mdpkg/auth/auth_middleware.gopkg/auth/identity.gopkg/auth/jwt_handler.gopkg/closer/closer.gopkg/closer/closer_test.gopkg/config/config.gopkg/config/dump.gopkg/config/health.gopkg/config/loader.gopkg/config/logging.gopkg/config/logging_test.gopkg/config/metrics.gopkg/config/tracing.gopkg/db/db_session/testcontainer.gotest/CLAUDE.mdtest/helper.gotest/integration/advisory_locks_test.gotest/integration/caller_identity_test.gotest/integration/integration_test.gotest/mocks/jwk_cert_server.gotest/registration.gotest/support/certs.jsontest/testdata/integration-config.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (12)
- cmd/hyperfleet-api/environments/registry/registry.go
- test/support/certs.json
- pkg/auth/identity.go
- cmd/hyperfleet-api/environments/e_integration_testing.go
- cmd/hyperfleet-api/environments/types.go
- pkg/db/db_session/testcontainer.go
- cmd/hyperfleet-api/environments/framework.go
- cmd/hyperfleet-api/environments/e_unit_testing.go
- cmd/hyperfleet-api/environments/e_production.go
- pkg/auth/auth_middleware.go
- cmd/hyperfleet-api/environments/framework_test.go
- cmd/hyperfleet-api/environments/e_development.go
🚧 Files skipped from review as they are similar to previous changes (40)
- cmd/hyperfleet-api/container/db.go
- charts/templates/configmap.yaml
- docs/deployment.md
- CONTRIBUTING.md
- cmd/hyperfleet-api/container/validation.go
- docs/testcontainers.md
- docs/authentication.md
- cmd/hyperfleet-api/servecmd/api_server.go
- pkg/config/dump.go
- pkg/config/config.go
- test/integration/advisory_locks_test.go
- test/testdata/integration-config.yaml
- charts/README.md
- test/integration/caller_identity_test.go
- pkg/auth/jwt_handler.go
- test/mocks/jwk_cert_server.go
- docs/development.md
- Makefile
- test/CLAUDE.md
- cmd/hyperfleet-api/container/container_test.go
- pkg/config/logging.go
- pkg/closer/closer.go
- cmd/hyperfleet-api/container/auth.go
- charts/values.yaml
- test/registration.go
- pkg/config/logging_test.go
- pkg/config/health.go
- cmd/hyperfleet-api/server/metrics_server.go
- cmd/hyperfleet-api/server/api_server.go
- cmd/hyperfleet-api/server/routes_entities.go
- pkg/config/loader.go
- test/integration/integration_test.go
- cmd/hyperfleet-api/container/container.go
- pkg/closer/closer_test.go
- cmd/hyperfleet-api/container/daos.go
- cmd/hyperfleet-api/servecmd/cmd.go
- cmd/hyperfleet-api/server/api_server_test.go
- docs/logging.md
- pkg/config/metrics.go
- cmd/hyperfleet-api/server/health_server.go
| **Request flow**: Router -> Middleware (logging, auth, transaction) -> Handler -> Service -> DAO -> GORM -> PostgreSQL | ||
|
|
||
| Create feature branches from `main`. PRs target `main`. | ||
| - **Startup wiring**: `servecmd.runServe` loads config -> `container.NewContainer(cfg)` -> `BuildAPIServer(...)` -> `server.NewRouterFromConfig` + `server.NewAPIServer` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the obsolete servecmd.runServe flow.
Line 66 still documents servecmd.runServe, but this PR replaces it with a linear composition root. Update the sequence to name the current entry point and include pkg/closer shutdown wiring. Otherwise, maintainers will follow a removed startup path.
As per PR objectives: “Replaces runServe with a linear composition root using pkg/closer for LIFO-ordered shutdown.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@AGENTS.md` at line 66, Update the Startup wiring entry in AGENTS.md to remove
the obsolete servecmd.runServe sequence, name the current linear
composition-root entry point shown by the PR, and include pkg/closer shutdown
wiring with LIFO ordering. Preserve the existing startup component sequence
where still applicable.
|
|
||
| func NewTracingConfig() *TracingConfig { | ||
| return &TracingConfig{ | ||
| Enabled: true, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Disable tracing by default.
Because NewApplicationConfig() starts from NewTracingConfig(), an unset Tracing.enabled value becomes true. This conflicts with the architecture contract requiring tracing to default to false and can enable telemetry egress and runtime overhead without explicit opt-in. Treat this as CWE-16, Insecure Configuration.
Proposed fix
- Enabled: true,
+ Enabled: false,As per linked architecture findings: HYPERFLEET_TRACING_ENABLED defaults to false. As per path instructions: configuration changes affect all deployments.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Enabled: true, | |
| Enabled: false, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/config/tracing.go` at line 19, Update the Enabled default in
NewTracingConfig to false so NewApplicationConfig produces disabled tracing
unless explicitly enabled, while preserving explicit configuration overrides.
Sources: Path instructions, Linked repositories
kuudori
force-pushed
the
HYPERFLEET-1371-remove-environments
branch
from
August 6, 2026 21:08
29bb6c5 to
efa7b9a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
cmd/hyperfleet-api/server/server.go (1)
52-52: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTLS files are still loaded twice.
Startvalidates the pair at Line 70.ServeTLSat Line 52 reads both files again. A change or removal between the two operations makesServeTLSfail afterclose(s.listening)publishes readiness. This is CWE-367.Store the validated
tls.CertificateinhttpServer.TLSConfig.Certificatesand callServeTLS(listener, "", "").🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/hyperfleet-api/server/server.go` at line 52, Update Start and the ServeTLS call to reuse the validated tls.Certificate instead of rereading certificate files: assign the validated certificate to httpServer.TLSConfig.Certificates, then call ServeTLS with empty certificate and key paths while preserving the existing readiness ordering.test/helper.go (1)
145-156: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSetup panics still leak the PostgreSQL testcontainer.
abortSetuppanics out ofonce.Doand therefore out ofNewHelper.TestMaincallsterminateContainer(ctx, pgContainer)only on the normal path, with nodefer. EveryabortSetupcall at Lines 149, 155, and 171 leaves the container running (CWE-772).helper.closernever ownspgContainer.Register the container termination in
TestMainwithdefer, or pass the termination function into the helper closer before the first failure point.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/helper.go` around lines 145 - 156, Ensure PostgreSQL testcontainer cleanup is registered before any setup can fail: update TestMain to defer terminateContainer(ctx, pgContainer), or register that termination with the helper closer before the abortSetup calls in NewHelper. Preserve cleanup for both normal execution and panics from abortSetup.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@cmd/hyperfleet-api/server/server.go`:
- Line 52: Update Start and the ServeTLS call to reuse the validated
tls.Certificate instead of rereading certificate files: assign the validated
certificate to httpServer.TLSConfig.Certificates, then call ServeTLS with empty
certificate and key paths while preserving the existing readiness ordering.
In `@test/helper.go`:
- Around line 145-156: Ensure PostgreSQL testcontainer cleanup is registered
before any setup can fail: update TestMain to defer terminateContainer(ctx,
pgContainer), or register that termination with the helper closer before the
abortSetup calls in NewHelper. Preserve cleanup for both normal execution and
panics from abortSetup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1e2f2117-957a-46e2-a58d-4209c0e27db5
⛔ Files ignored due to path filters (1)
test/support/jwt_ca.pemis excluded by!**/*.pem
📒 Files selected for processing (57)
AGENTS.mdCLAUDE.mdCONTRIBUTING.mdMakefilecharts/README.mdcharts/templates/configmap.yamlcharts/values.yamlcmd/hyperfleet-api/container/auth.gocmd/hyperfleet-api/container/container.gocmd/hyperfleet-api/container/container_test.gocmd/hyperfleet-api/container/daos.gocmd/hyperfleet-api/container/db.gocmd/hyperfleet-api/container/validation.gocmd/hyperfleet-api/environments/e_development.gocmd/hyperfleet-api/environments/e_integration_testing.gocmd/hyperfleet-api/environments/e_production.gocmd/hyperfleet-api/environments/e_unit_testing.gocmd/hyperfleet-api/environments/framework.gocmd/hyperfleet-api/environments/framework_test.gocmd/hyperfleet-api/environments/registry/registry.gocmd/hyperfleet-api/environments/types.gocmd/hyperfleet-api/servecmd/api_server.gocmd/hyperfleet-api/servecmd/cmd.gocmd/hyperfleet-api/server/api_server.gocmd/hyperfleet-api/server/api_server_test.gocmd/hyperfleet-api/server/health_server.gocmd/hyperfleet-api/server/metrics_server.gocmd/hyperfleet-api/server/routes_entities.gocmd/hyperfleet-api/server/server.godocs/authentication.mddocs/deployment.mddocs/development.mddocs/logging.mddocs/testcontainers.mdpkg/auth/auth_middleware.gopkg/auth/identity.gopkg/auth/jwt_handler.gopkg/closer/closer.gopkg/closer/closer_test.gopkg/config/config.gopkg/config/dump.gopkg/config/health.gopkg/config/loader.gopkg/config/logging.gopkg/config/logging_test.gopkg/config/metrics.gopkg/config/tracing.gopkg/db/db_session/testcontainer.gotest/CLAUDE.mdtest/helper.gotest/integration/advisory_locks_test.gotest/integration/caller_identity_test.gotest/integration/integration_test.gotest/mocks/jwk_cert_server.gotest/registration.gotest/support/certs.jsontest/testdata/integration-config.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (12)
- cmd/hyperfleet-api/environments/framework_test.go
- cmd/hyperfleet-api/environments/framework.go
- cmd/hyperfleet-api/environments/e_production.go
- cmd/hyperfleet-api/environments/e_integration_testing.go
- cmd/hyperfleet-api/environments/e_unit_testing.go
- test/support/certs.json
- cmd/hyperfleet-api/environments/types.go
- pkg/auth/auth_middleware.go
- cmd/hyperfleet-api/environments/registry/registry.go
- cmd/hyperfleet-api/environments/e_development.go
- pkg/db/db_session/testcontainer.go
- pkg/auth/identity.go
🚧 Files skipped from review as they are similar to previous changes (38)
- docs/logging.md
- test/integration/advisory_locks_test.go
- pkg/config/config.go
- Makefile
- cmd/hyperfleet-api/container/db.go
- test/testdata/integration-config.yaml
- pkg/config/dump.go
- docs/authentication.md
- pkg/closer/closer.go
- pkg/config/health.go
- docs/deployment.md
- CONTRIBUTING.md
- charts/README.md
- CLAUDE.md
- cmd/hyperfleet-api/container/auth.go
- pkg/config/metrics.go
- charts/templates/configmap.yaml
- docs/testcontainers.md
- test/CLAUDE.md
- test/registration.go
- pkg/auth/jwt_handler.go
- test/mocks/jwk_cert_server.go
- docs/development.md
- test/integration/caller_identity_test.go
- cmd/hyperfleet-api/server/routes_entities.go
- cmd/hyperfleet-api/container/container_test.go
- test/integration/integration_test.go
- pkg/config/tracing.go
- pkg/config/logging.go
- pkg/closer/closer_test.go
- cmd/hyperfleet-api/container/validation.go
- cmd/hyperfleet-api/container/daos.go
- pkg/config/logging_test.go
- cmd/hyperfleet-api/servecmd/cmd.go
- cmd/hyperfleet-api/server/api_server.go
- cmd/hyperfleet-api/server/api_server_test.go
- charts/values.yaml
- cmd/hyperfleet-api/servecmd/api_server.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
environmentsframework and its registry in favor of direct container-based dependency injectionrunServewith a linear composition root usingpkg/closerfor LIFO ordered shutdownHYPERFLEET_TRACING_ENABLED,OTEL_SERVICE_NAME) from rawos.Getenvinto the Viper config system as a top-levelTracingconfig sectionHYPERFLEET_LOGGING_OTEL_ENABLED/HYPERFLEET_LOGGING_OTEL_SAMPLING_RATEwarning blockstracingEnabledparam fromBuildAPIServeranddbPingTimeoutfromNewHealthServer- both already available via cfgTest plan
make verify-allpasses (1436 tests, lint, vet)make test-helmpasses (21 chart tests)make test-integrationpasses